`accessible_by` may crash when a `default_scope` is used #600

ghiculescu · 2019-08-25T22:25:01Z

Problem

If you have a default_scope that sets ordering, and define a permission that checks for records via multiple tables, and use a has_many :through, and run Postgres... then on version 3.x, accessible_by may crash. The attached test suite shows how; the tests that call alex.blog_post_comments fail with:

     ActiveRecord::StatementInvalid:
       PG::InvalidColumnReference: ERROR:  for SELECT DISTINCT, ORDER BY expressions must appear in select list
       LINE 1: ... ORDER BY "blog_post_comments"."created_at" DESC, "blog_post...

This regressed in #512

Previous query (worked fine):

SELECT "blog_post_comments"."id" AS t0_r0, "blog_post_comments"."blog_post_id" AS t0_r1, "blog_post_comments"."body" AS t0_r2, "blog_post_comments"."created_at" AS t0_r3, "blog_post_comments"."updated_at" AS t0_r4, "blog_posts_blog_post_comments"."id" AS t1_r0, "blog_posts_blog_post_comments"."blog_author_id" AS t1_r1, "blog_posts_blog_post_comments"."title" AS t1_r2, "blog_posts_blog_post_comments"."created_at" AS t1_r3, "blog_posts_blog_post_comments"."updated_at" AS t1_r4, "blog_authors"."id" AS t2_r0, "blog_authors"."name" AS t2_r1, "blog_authors"."created_at" AS t2_r2, "blog_authors"."updated_at" AS t2_r3 FROM "blog_post_comments" LEFT OUTER JOIN "blog_posts" "blog_posts_blog_post_comments" ON "blog_posts_blog_post_comments"."id" = "blog_post_comments"."blog_post_id" LEFT OUTER JOIN "blog_authors" ON "blog_authors"."id" = "blog_posts_blog_post_comments"."blog_author_id" INNER JOIN "blog_posts" ON "blog_post_comments"."blog_post_id" = "blog_posts"."id" WHERE "blog_posts"."blog_author_id" = 1 AND "blog_authors"."id" = 1 ORDER BY "blog_post_comments"."created_at" DESC, "blog_posts"."title" ASC

New query (crashes):

SELECT DISTINCT "blog_post_comments".* FROM "blog_post_comments" INNER JOIN "blog_posts" ON "blog_post_comments"."blog_post_id" = "blog_posts"."id" LEFT OUTER JOIN "blog_posts" "blog_posts_blog_post_comments" ON "blog_posts_blog_post_comments"."id" = "blog_post_comments"."blog_post_id" LEFT OUTER JOIN "blog_authors" ON "blog_authors"."id" = "blog_posts_blog_post_comments"."blog_author_id" WHERE "blog_posts"."blog_author_id" = 1 AND "blog_authors"."id" = 1 ORDER BY "blog_post_comments"."created_at" DESC, "blog_posts"."title" ASC

After writing this up I realised there was a much simpler replication case reported here: #508 (comment)

Also I know that this is mentioned in the updating notes but in my opinion cancancan should be able to handle this for the user.

Solution

Okay so I kind of hate this PR but I haven't found a better solution. There's a few things going on here.

add_joins_to_relation. Relation#left_joins does not work super well in AR 5.0 and 5.1; it's failing on a bunch of the tests added in this PR. Thus I'm not using it for older Rails versions; for those I'm falling back to how joins worked before Kaspernj optimised queries with left joins #512.
extract_ids_and_requery has like a million comments explaining what it does. It means we'll be running an extra query fairly often so I'm keen for feedback on if this is actually worth doing.

Basically I hate this solution but I can't find another way of getting it to work if we keep left_joins. And if we ditch left_joins, there's still things that don't work. For example, there's some tests that do Post.accessible_by(ability).select('title as mytitle') that only work if you use left_joins (they were introduced after #512).

If you have a `default_scope` that sets ordering, and define a permission that checks for records via multiple tables, and use a `has_many :through`, and run Postgres... then on version 3.x, `accessible_by` may crash. The attached test suite shows how; the tests that call `alex.blog_post_comments` fail with: ``` ActiveRecord::StatementInvalid: PG::InvalidColumnReference: ERROR: for SELECT DISTINCT, ORDER BY expressions must appear in select list LINE 1: ... ORDER BY "blog_post_comments"."created_at" DESC, "blog_post... ^ : SELECT DISTINCT "blog_post_comments".* FROM "blog_post_comments" INNER JOIN "blog_posts" ON "blog_post_comments"."blog_post_id" = "blog_posts"."id" LEFT OUTER JOIN "blog_posts" "blog_posts_blog_post_comments" ON "blog_posts_blog_post_comments"."id" = "blog_post_comments"."blog_post_id" LEFT OUTER JOIN "blog_authors" ON "blog_authors"."id" = "blog_posts_blog_post_comments"."blog_author_id" WHERE "blog_posts"."blog_author_id" = $1 AND "blog_authors"."id" = $2 ORDER BY "blog_post_comments"."created_at" DESC, "blog_posts"."title" ASC ``` This regressed in #512

The fixed query looks like this: ``` SELECT DISTINCT blog_post_comments.created_at AS active_record_query_fixer_0, blog_posts.title AS active_record_query_fixer_1, blog_post_comments.* FROM "blog_post_comments" INNER JOIN "blog_posts" ON "blog_post_comments"."blog_post_id" = "blog_posts"."id" LEFT OUTER JOIN "blog_posts" "blog_posts_blog_post_comments" ON "blog_posts_blog_post_comments"."id" = "blog_post_comments"."blog_post_id" LEFT OUTER JOIN "blog_authors" ON "blog_authors"."id" = "blog_posts_blog_post_comments"."blog_author_id" WHERE "blog_posts"."blog_author_id" = 1 AND "blog_authors"."id" = 1 ORDER BY "blog_post_comments"."created_at" DESC, "blog_posts"."title" ASC ``` (note extra columns in the `SELECT DISTINCT`) I'm not sure on cancancan's policy on adding extra gems, so I've just copy and pasted the relevant methods in - happy to tidy up.

ghiculescu · 2019-08-25T22:51:39Z

@kaspernj @coorasse what do you think?

ghiculescu · 2019-08-25T23:12:31Z

Based on the CI output it looks like #512 also introduced an issue Rails 5.0 and 5.1 when defining abilities with nested hashes?

If you reverse the changes made in #512 then the new tests here I've added all pass. Because of this, I'm proposing we only use left_joins from AR 5.2 onwards: 1997832. Annoying, this is causing plenty of other things to fail.

Before I continue down this path I'm keen for feedback on if the 2 changes I'm proposing in this PR are worthwhile, and if it makes more sense to split them into separate PRs. Here's the changelog for reference: a9642b2

kaspernj · 2019-08-26T06:27:13Z

Great work! I am very happy you have found my code useful, and I cant wait to use this instead of my own gem :-)

ghiculescu · 2019-08-30T22:25:50Z

@kaspernj @coorasse keen for some opinions on this one. See updated PR body.

coorasse · 2019-09-01T16:34:44Z

Hi @ghiculescu . Thanks for the time you put in this PR.
I quickly tried to pull your branch locally and run all the tests you added, removing the patch you applied.
The result is that the tests
rspec ./spec/cancan/model_adapters/active_record_5_adapter_with_default_scopes_spec.rb:147 and
rspec ./spec/cancan/model_adapters/active_record_5_adapter_with_default_scopes_spec.rb:140 fail because of the following error:

ActiveRecord::StatementInvalid:
       SQLite3::SQLException: ambiguous column name: blog_posts.blog_author_id: SELECT DISTINCT "blog_post_comments".* FROM "blog_post_comments" INNER JOIN "blog_posts" ON "blog_post_comments"."blog_post_id" = "blog_posts"."id" LEFT OUTER JOIN "blog_posts" ON "blog_posts"."id" = "blog_post_comments"."blog_post_id" WHERE "blog_posts"."blog_author_id" = ? AND "blog_posts"."blog_author_id" = ? ORDER BY "blog_post_comments"."created_at" DESC, "blog_posts"."title" ASC

I therefore see an error in the examples you provided but which is not related to the default_scope. The same error appears if I remove the default_scope.
The problem is due to a poor management of joins and left_joins when used at the same time (as you highlighted already) because of names clashing (no alias is added to one of the joins).
At the same time I can also see that in AR >= 5.2 the aliases are added and the tests pass but the left join could be avoided.

Here are my considerations at the moment:

I think a better test to reproduce the error is necessary. By removing the patch and running the tests I could see any PG::InvalidColumnReference error triggered.
I think an improvement could be made for all the versions when a join is already present (read: do not generate another left join). This is another matter and can be tackled in a separate PR. this could also solve the issue on version 5.0

Can you help me to reproduce your issue? If I have missed something please point to me. I do not absolutely exclude that I might have missed something. 😄

Thanks 🙏

coorasse · 2019-09-01T16:37:09Z

One additional note regarding your comment:

Also I know that this is mentioned in the updating notes but in my opinion cancancan should be able to handle this for the user.
There are so maaaany different situations that I think it would be better to tackle them with code examples rather than trying to manage them all.
At some point a nested join solves all the issue 😄

ghiculescu · 2019-09-03T17:17:11Z

@coorasse here's how to replicate: https://gist.github.com/ghiculescu/a3a713134f18e351c7ba521377146014

Take the tests I've added in this PR and nothing else, and run DB=postgres bundle exec appraisal activerecord_5.2.2 rspec spec/cancan/model_adapters/active_record_5_adapter_with_default_scopes_spec.rb.

Note that the tests don't fail if you're using sqlite. But I use postgres in production, that's how I came across the issue.

ghiculescu · 2019-09-06T16:21:47Z

@coorasse any more thoughts on this one?

If you aren't happy with this style of fix I can go back and work around this issue in my code. Keen to get a resolution one way or another as this is blocking our rails 6 update.

ghiculescu · 2019-11-19T21:47:59Z

We have ended up working around this in our codebase.

coorasse and others added 10 commits January 16, 2018 09:36

Merge branch 'release/2.1.3'

b602439

Merge branch 'develop'

0e42c4e

Merge branch 'release/2.1.4'

68772b4

Merge branch 'release/2.2.0'

4b1008e

Merge branch 'release/2.3.0'

1760baa

Merge branch 'release/3.0.0'

e10c3c9

Merge branch 'release/3.0.1'

61bfbc1

lint

a277eb1

ghiculescu marked this pull request as ready for review August 25, 2019 22:51

ghiculescu added 2 commits August 25, 2019 18:14

simplify ability definition

617b871

only run #left_joins in AR 5.2+

1997832

This was referenced Aug 25, 2019

Kaspernj optimised queries with left joins #512

Merged

accessible_by with hash of conditions causes Not unique table/alias #596

Closed

Use include with references to build relations instead of left_join #593

Closed

ghiculescu added 4 commits August 25, 2019 18:43

add to changelog

a9642b2

ugh, this solution totally breaks counts. add tests.

e8faabc

better solution that doesn't overuse distinct, or add extra selects

e130493

lint

3f3fed5

only check SQL on latest AR since it can differ between AR versions

9ca01d9

coorasse self-assigned this Aug 26, 2019

coorasse added the bug label Aug 26, 2019

ghiculescu added 4 commits August 30, 2019 16:39

Merge branch 'master' into feature/default_scope_distinct_handling

89531eb

sooooo fixtures are loaded lazily it turns out

c66ab2b

i'm calling it now. you can only do this from 5.2

50f5c0e

these tests arent interesting anymore

de006fd

fsateler mentioned this pull request Nov 19, 2019

Generate inner queries instead of join+distinct #605

Closed

ghiculescu closed this Nov 19, 2019

ghiculescu deleted the feature/default_scope_distinct_handling branch October 12, 2020 21:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`accessible_by` may crash when a `default_scope` is used #600

`accessible_by` may crash when a `default_scope` is used #600

ghiculescu commented Aug 25, 2019 •

edited

Loading

ghiculescu commented Aug 25, 2019

ghiculescu commented Aug 25, 2019 •

edited

Loading

kaspernj commented Aug 26, 2019

ghiculescu commented Aug 30, 2019 •

edited

Loading

coorasse commented Sep 1, 2019

coorasse commented Sep 1, 2019

ghiculescu commented Sep 3, 2019

ghiculescu commented Sep 6, 2019

ghiculescu commented Nov 19, 2019

accessible_by may crash when a default_scope is used #600

accessible_by may crash when a default_scope is used #600

Conversation

ghiculescu commented Aug 25, 2019 • edited Loading

Problem

Solution

ghiculescu commented Aug 25, 2019

ghiculescu commented Aug 25, 2019 • edited Loading

kaspernj commented Aug 26, 2019

ghiculescu commented Aug 30, 2019 • edited Loading

coorasse commented Sep 1, 2019

coorasse commented Sep 1, 2019

ghiculescu commented Sep 3, 2019

ghiculescu commented Sep 6, 2019

ghiculescu commented Nov 19, 2019

`accessible_by` may crash when a `default_scope` is used #600

`accessible_by` may crash when a `default_scope` is used #600

ghiculescu commented Aug 25, 2019 •

edited

Loading

ghiculescu commented Aug 25, 2019 •

edited

Loading

ghiculescu commented Aug 30, 2019 •

edited

Loading